Add unit tests for analytics, exercise, routine, target managers, and… - #45
Merged
Conversation
… target calculators - Created `analytics_manager_test.dart` to test session indexing, growth model updates, recommendations, and volume progression. - Added `exercise_manager_test.dart` for loading exercises, adding custom exercises, and filtering/searching functionalities. - Implemented `routine_manager_test.dart` to validate routine loading, creation, updating, and deletion. - Developed `target_calculator_test.dart` to ensure correct calculations for reps, weight, and volume targets. - Introduced `target_manager_test.dart` to test target creation, updates, deletions, and retrievals for exercises.
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a Health Connect bulk-sync UI and a manager API to trigger syncing of all unsynced sessions, tightens segment-building fallback logic in HealthConnectService to avoid zero-duration final segments, and adds comprehensive unit tests for multiple managers and calculator strategies. ChangesHealth Connect Sync + Segment Robustness
Manager / Calculator Test Suites
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing [Slack Agent](https://www.coderabbit.ai/agent): Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 41 seconds.Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
+ Coverage 29.13% 32.18% +3.04%
==========================================
Files 33 36 +3
Lines 5999 6187 +188
==========================================
+ Hits 1748 1991 +243
+ Misses 4251 4196 -55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/test/analytics_manager_test.dart`:
- Around line 7-13: Change the helper builders _session and _exercise to use
only named parameters (make id, exerciseId, muscleGroupId, and other args named)
because they have 3+ arguments; update every call site that uses positional
invocation (e.g. _session('s1','ex1',...) and _exercise('ex1','chest')) to use
named invocation (e.g. _session(id: 's1', exerciseId: 'ex1', ...), _exercise(id:
'ex1', muscleGroupId: 'chest')), ensuring default values and required
annotations are preserved (e.g. keep required DateTime date) and adjust any
tests referencing those helpers accordingly.
- Around line 348-357: The test uses DateTime.now() which makes getQuickStats
non-deterministic; change the session timestamp passed to
mockStorage.addMockSession/_session (the call with _session('s1', 'ex1', date:
DateTime.now())) to a fixed, deterministic DateTime (e.g., a hard-coded UTC
constant) so the test always uses the same date when manager.getQuickStats() is
evaluated.
- Around line 141-177: The test "trains a model for every exercise present in
sessions" only asserts that a model for 'ex1' was created; update the test to
also assert that manager.getGrowthModel('ex2') is not null after calling
manager.trainAllGrowthModels(sessions), so both exerciseIds present in the
sessions (ex1 and ex2) are verified as having models trained; reference the
existing sessions array and the manager.trainAllGrowthModels(...) invocation to
add the additional expectation.
In `@workout-logger/test/exercise_manager_test.dart`:
- Around line 127-147: Update the two tests ('throws ArgumentError for empty
name' and 'throws ArgumentError for invalid category') to use await expectLater
instead of expect so the async matcher is awaited and lint/unnecessary_async is
satisfied: call await expectLater(manager.addCustomExercise(name: ..., category:
..., primaryMuscleGroupId: ...), throwsArgumentError) for each case (referencing
manager.addCustomExercise) so the Future is awaited and failures report
correctly.
In `@workout-logger/test/routine_manager_test.dart`:
- Around line 93-99: The test adds an orphan routine via manager.updateRoutine
but only asserts in-memory state; add an assertion that the routine was
persisted to storage by verifying the fake/mock storage contains the routine or
that the storage save method was called (reference the manager.updateRoutine
call and the internal _storage.saveRoutine behavior) — e.g., after awaiting
manager.updateRoutine(orphan) assert the storage mock recorded a save for id
'orphan' or that storage.getRoutine('orphan') returns the saved Routine,
ensuring persistence is exercised as well as manager.routines.
In `@workout-logger/test/target_calculator_test.dart`:
- Around line 54-59: The test points out that WeightTargetCalculator.calculate
(in target_calculator.dart) can throw a StateError because it calls .reduce() on
the mapped sets when a matching ExerciseLog has an empty sets list; update the
calculate implementation to guard for empty iterables by replacing the unsafe
.reduce(...) with .fold(initialValue, ...) or by checking for empty sets before
reducing so an empty ExerciseLog yields 0, and add the suggested test case that
builds a WorkoutSession with ExerciseLog(exerciseId: 'ex1', sets: []) to assert
calculator.calculate('ex1', sessions) returns 0 to prevent regressions.
- Around line 162-176: The test helper class _AlwaysReturns42 has no instance
fields so add a const constructor (e.g., const _AlwaysReturns42();) to the class
and replace its non-constant instantiations with const instances; update the
TargetCalculatorFactory.registerCalculator calls that currently use
_AlwaysReturns42() (for example the 'custom' and 'extra' registrations and any
other test sites) to use const _AlwaysReturns42() so the tests follow the "use
const constructors wherever possible" guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79883ad2-1c9b-4a26-ac1c-82ac955e4d86
📒 Files selected for processing (9)
workout-logger/lib/screens/history_screen.dartworkout-logger/lib/services/health_connect_service.dartworkout-logger/lib/services/managers/history_manager.dartworkout-logger/test/active_workout_manager_test.dartworkout-logger/test/analytics_manager_test.dartworkout-logger/test/exercise_manager_test.dartworkout-logger/test/routine_manager_test.dartworkout-logger/test/target_calculator_test.dartworkout-logger/test/target_manager_test.dart
Comment on lines
+54
to
+59
| group('WeightTargetCalculator', () { | ||
| final calculator = WeightTargetCalculator(); | ||
|
|
||
| test('returns 0 when no sessions', () { | ||
| expect(calculator.calculate('ex1', []), 0); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing edge case: WeightTargetCalculator crashes on a matched log with empty sets
WeightTargetCalculator.calculate() calls .reduce() on log.sets.map(…) (see target_calculator.dart lines 60–62). Dart's own docs note "Consider using fold if the iterable can be empty" — reduce throws StateError when the iterable is empty. The existing "returns 0 when no sessions" test passes an empty sessions list, which bypasses the inner reduce entirely. A session that matches the exercise ID but contains zero sets will crash the production calculator. Adding a test here would surface this latent bug.
➕ Suggested test to add
test('returns 0 when matched exercise log has empty sets', () {
final sessions = [
WorkoutSession(
id: 's1',
date: DateTime(2024, 1, 1),
duration: 30,
exercises: [ExerciseLog(exerciseId: 'ex1', sets: [])],
),
];
// Currently throws StateError — exposes the unguarded reduce() in WeightTargetCalculator
expect(calculator.calculate('ex1', sessions), 0);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workout-logger/test/target_calculator_test.dart` around lines 54 - 59, The
test points out that WeightTargetCalculator.calculate (in
target_calculator.dart) can throw a StateError because it calls .reduce() on the
mapped sets when a matching ExerciseLog has an empty sets list; update the
calculate implementation to guard for empty iterables by replacing the unsafe
.reduce(...) with .fold(initialValue, ...) or by checking for empty sets before
reducing so an empty ExerciseLog yields 0, and add the suggested test case that
builds a WorkoutSession with ExerciseLog(exerciseId: 'ex1', sets: []) to assert
calculator.calculate('ex1', sessions) returns 0 to prevent regressions.
…ics and exercise managers
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workout-logger/lib/services/strategies/target_calculator.dart (1)
36-82: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd
constconstructors to all three calculator classes.None of
RepsTargetCalculator,WeightTargetCalculator, orVolumeTargetCalculatordeclare an explicit constructor, so they cannot be instantiated withconst— both the factory map and the test-filecalculatordeclarations are therefore non-const. Adding trivialconstconstructors fixes this across both files.♻️ Proposed fix
class RepsTargetCalculator implements TargetCalculatorStrategy { + const RepsTargetCalculator(); `@override` double calculate(String exerciseId, List<WorkoutSession> sessions) {class WeightTargetCalculator implements TargetCalculatorStrategy { + const WeightTargetCalculator(); `@override` double calculate(String exerciseId, List<WorkoutSession> sessions) {class VolumeTargetCalculator implements TargetCalculatorStrategy { + const VolumeTargetCalculator(); `@override` double calculate(String exerciseId, List<WorkoutSession> sessions) {Then in the factory:
static final Map<String, TargetCalculatorStrategy> _strategies = { - 'reps': RepsTargetCalculator(), - 'weight': WeightTargetCalculator(), - 'volume': VolumeTargetCalculator(), + 'reps': const RepsTargetCalculator(), + 'weight': const WeightTargetCalculator(), + 'volume': const VolumeTargetCalculator(), };As per coding guidelines: "Use
constconstructors wherever possible."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workout-logger/lib/services/strategies/target_calculator.dart` around lines 36 - 82, The three strategy classes (RepsTargetCalculator, WeightTargetCalculator, VolumeTargetCalculator) lack explicit constructors so they cannot be instantiated as const; add trivial const constructors to each class (e.g. const RepsTargetCalculator(); const WeightTargetCalculator(); const VolumeTargetCalculator();) so callers and factory maps can use const instances and follow the "use const constructors wherever possible" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workout-logger/test/target_calculator_test.dart`:
- Around line 92-114: The VolumeTargetCalculator tests are missing coverage that
ensures sessions for other exercises are ignored; add a test in the
VolumeTargetCalculator group that creates sessions for 'ex1' and a different
exercise (e.g., 'ex2') and asserts calculator.calculate('ex1', sessions) only
counts volume from 'ex1' (use the same _session helper and WorkoutSet entries as
in other tests). This verifies the shared filter (e.g.,
_getExerciseLogsForExercise) is applied for VolumeTargetCalculator.calculate and
prevents regressions.
---
Outside diff comments:
In `@workout-logger/lib/services/strategies/target_calculator.dart`:
- Around line 36-82: The three strategy classes (RepsTargetCalculator,
WeightTargetCalculator, VolumeTargetCalculator) lack explicit constructors so
they cannot be instantiated as const; add trivial const constructors to each
class (e.g. const RepsTargetCalculator(); const WeightTargetCalculator(); const
VolumeTargetCalculator();) so callers and factory maps can use const instances
and follow the "use const constructors wherever possible" guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9908042-020b-40b9-86fe-3709a38a520b
📒 Files selected for processing (5)
workout-logger/lib/services/strategies/target_calculator.dartworkout-logger/test/analytics_manager_test.dartworkout-logger/test/exercise_manager_test.dartworkout-logger/test/routine_manager_test.dartworkout-logger/test/target_calculator_test.dart
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… target calculators
analytics_manager_test.dartto test session indexing, growth model updates, recommendations, and volume progression.exercise_manager_test.dartfor loading exercises, adding custom exercises, and filtering/searching functionalities.routine_manager_test.dartto validate routine loading, creation, updating, and deletion.target_calculator_test.dartto ensure correct calculations for reps, weight, and volume targets.target_manager_test.dartto test target creation, updates, deletions, and retrievals for exercises.Summary by CodeRabbit
New Features
Bug Fixes
Tests